stream: group bool fields at tail of serverStream to eliminate false sharing with mu - #9361
Merged
mbissa merged 2 commits intoAug 27, 2026
Merged
Conversation
…ent padding Two bool fields in serverStream were placed between aligned fields, each forcing alignment padding before the next field: recvFirstMsg @ 168 7 B padding (before int, align 8) serverHeaderBinlogged @ 240 3 B padding (before sync.Mutex, align 4) Grouping both bools at the tail reduces serverStream from 256 B to 248 B, saving 8 B per server-side RPC stream unconditionally. Fixes grpc#9349
gidotencate
force-pushed
the
stream-group-bools-server-stream
branch
from
August 24, 2026 16:17
8ba065c to
ea29db8
Compare
Contributor
Author
|
@easwars @mbissa Could one of you assign this PR (and linked issue #9349) to @gidotencate? I don't have write access to assign myself. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9361 +/- ##
==========================================
+ Coverage 87.45% 87.50% +0.05%
==========================================
Files 425 425
Lines 30297 30303 +6
==========================================
+ Hits 26496 26517 +21
+ Misses 3801 3786 -15
🚀 New features to boost your workflow:
|
eshitachandwani
approved these changes
Aug 26, 2026
eshitachandwani
left a comment
Member
There was a problem hiding this comment.
LGTM , adding @mbissa as a second reviewer.
gidotencate
force-pushed
the
stream-group-bools-server-stream
branch
from
August 26, 2026 10:01
6058ba1 to
869d5bc
Compare
Contributor
Author
|
Done, removed in the latest commit. |
serverHeaderBinlogged (written unsynchronised in SendHeader and Send) and mu shared cache line 3 (192–255) after the initial tail-grouping commit. Concurrent writes to serverHeaderBinlogged invalidate the cache line before every mu.Lock(), adding ~5–21× overhead under concurrent load. Moving mu to immediately before trInfo places it at offset 184 (cache line 2: 128–191). The tail bools remain at offset 240+ (cache line 3), so stressor writes no longer affect mu.Lock() latency. Benchmark before this commit (serverHeaderBinlogged stressor): stressors=0 3.9 ns stressors=1 20.1 ns (5.2×) stressors=4 79.9 ns (20.4×) Benchmark after (different cache lines): stressors=0 3.7 ns stressors=1 3.8 ns (~1×) stressors=4 3.7 ns (~1×) No size change: serverStream remains 248 B. Fixes grpc#9349
gidotencate
force-pushed
the
stream-group-bools-server-stream
branch
from
August 26, 2026 11:00
869d5bc to
b91c821
Compare
Contributor
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request reorganizes the fields of the serverStream struct in stream.go to group boolean fields at the end, eliminating alignment padding and reducing the struct's memory footprint. There are no review comments, and we have no additional feedback to provide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reorder `serverStream` fields so both bool fields are grouped at the tail of the struct, and move `mu` to a separate cache line from the tail bools.
Why
`serverStream` had 2 `bool` fields placed between aligned fields, each forcing alignment padding before the next field:
Total: 8 B wasted. Grouping both bools at the tail eliminates the interior padding, reducing `serverStream` from 256 B → 248 B, saving 8 B per server-side RPC stream unconditionally.
Additionally, `serverHeaderBinlogged` is written unsynchronised in `SendHeader` and `Send` (it is documented as not needing synchronization). In the tail-grouped layout, the bools land at offset 240+ — which was the same cache line as `mu` (offset 232, cache line 3: 192–255). Concurrent writes to `serverHeaderBinlogged` invalidate the cache line before every `mu.Lock()`, adding 5–21× latency overhead under concurrent load.
Moving `mu` to immediately before `trInfo` (which it guards) places it at offset 184 (cache line 2: 128–191). The tail bools remain at offset 240+ (cache line 3), so the false-sharing is eliminated. No size change from this step — `serverStream` stays at 248 B.
See benchmarks in #9349.
Benchmark — `mu.Lock()` latency with stressor goroutines writing `serverHeaderBinlogged` concurrently:
Code clarity
Both bools are unguarded. The bool group uses a single `// Not guarded by mu` section, preserving the existing per-field comments. `mu` is placed immediately before `trInfo` (which it guards) with a comment explaining the cache-line placement.
Closes #9349
RELEASE NOTES: